test(api/mcp): cover full heatmap save/get/update round-trip#2199
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
PR ReviewTest-only PR — adds four integration tests covering full heatmap save/get/update round-trip, two update-side source-kind gates, and a mixed-tile round-trip. No production code changes. ✅ No critical issues found. Minor notes (non-blocking):
|
E2E Test Results✅ All tests passed • 175 passed • 3 skipped • 1221s
Tests ran across 4 shards in parallel. |
| @@ -0,0 +1,170 @@ | |||
| import { mcpTilesParam } from '../schemas'; | |||
|
|
|||
| describe('mcpTilesParam: heatmap tiles', () => { | |||
There was a problem hiding this comment.
These tests only validate that zod works - it would be helpful to test that they get added to the dashboard successfully (*more of an integration e2e test)
Brandon flagged on #2199 that the schema unit tests only assert that zod accepts the shape and don't prove that an agent-issued hyperdx_save_dashboard call actually persists a heatmap with every MCP-specific field. The bare-minimum heatmap test that lands with the #2200 heatmap external-API work covers only the required valueExpression; this test drives the full round-trip with heatmapScaleType, countExpression, numberFormat, where, and whereLanguage all set, then mutates them on PUT and re-reads to confirm the new values stick. Drive-by: drops an em-dash from a sibling test comment to satisfy the prose-lint hook on this file.
e583fc6 to
73aaf3b
Compare
| }); | ||
| }); | ||
|
|
||
| // Brandon flagged on #2199 that the schema unit tests only assert that |
There was a problem hiding this comment.
these comments aren't very helpful in the long run - we should decrease the amount of comments on the PRs. If the test description is good enough, no comment should be needed, or 1 line comment at most.
#2200) ## Summary Heatmap was the only builder-mode display type that did not round-trip through the external dashboards API. The serializer dropped it into the "unsupported" fall-through, so creating, fetching, and updating heatmap tiles via `/api/v2/dashboards` lost the config. This wires up heatmap end-to-end on the external API: a dedicated select-item schema, an explicit case in both serialization directions, OpenAPI JSDoc, and tests. Follow-up to #2107 (review feedback from @pulpdrew, who asked whether we had a follow-up ticket to update the external API for the new visualization type). ## What's in the diff - **Zod schemas** (`packages/api/src/utils/zod.ts`): a heatmap select-item schema that exposes the literal `aggFn: "heatmap"` plus `valueExpression`, optional `countExpression`, `alias`, `heatmapScaleType`; and a heatmap chart-config schema with optional `groupBy` and `numberFormat`. Heatmap is added to the **builder** discriminated union only. - **Conversion utilities** (`packages/api/src/routers/external-api/v2/utils/dashboards.ts`): - Builder serializer: replaces the old `case DisplayType.Heatmap:` fall-through with an explicit case that reads heatmap-specific fields off `config.select[0]` and emits `aggFn: "heatmap"` on the external surface. - Builder deserializer: new `case 'heatmap':` mirroring the Pie pattern; maps the external `aggFn: "heatmap"` back to the internal `aggFn: "count"` that the editor form persists, while preserving `countExpression`, `alias`, and `heatmapScaleType` on the select item. - The raw-SQL switch is intentionally left untouched: heatmap stays in the unsupported fall-through there because heatmap rendering requires `isBuilderChartConfig`. - **OpenAPI JSDoc** (`packages/api/src/routers/external-api/v2/dashboards.ts`): `HeatmapSelectItem` and `HeatmapBuilderChartConfig` components, and `heatmap` added to the `TileConfig` `oneOf` and discriminator mapping. - **Tests** (`packages/api/src/routers/external-api/__tests__/dashboards.test.ts`): heatmap added to the existing "round-trip all supported chart types" tests for both POST and PUT, plus an explicit rejection test confirming raw-SQL heatmap tiles return 400. - **`packages/api/openapi.json`**: regenerated. ## Notes for review - No raw-SQL heatmap variant. PR #2107 made heatmap builder-only and `DBDashboardPage.tsx` requires `isBuilderChartConfig` for heatmap rendering, so the raw-SQL fall-through stays. - `heatmapScaleType` and `countExpression` are persisted on the per-select-item level (via `DerivedColumnSchema` in `packages/common-utils/src/types.ts`), not on the chart config root. The form binds them as `series.0.heatmapScaleType` / `series.0.countExpression`. The schema and conversion utilities follow that. - The aggFn translation (external `"heatmap"` ↔ internal `"count"`) keeps the saved Mongo document identical to what the editor form produces, so heatmap tiles created via the API render the same way as ones created via the UI. ## Tier Lands as `review/tier-4` because anything under `packages/api/src/routers/external-api/` is on the critical-path list. Diff is ~250 prod lines (most of it OpenAPI JSDoc and Zod boilerplate); no schema migrations or auth changes. ## Test plan - [x] `make ci-lint` (yarn lint, tsc --noEmit, OpenAPI lint) - [x] `make ci-unit` (common-utils + app) - [ ] CI runs the full integration suite (heatmap round-trip POST + PUT + raw-SQL rejection); local `make dev-int` requires Docker BuildKit which isn't available on this host. ## Deep-review carryover (2026-05-07) - **Resolved here**: P0/P1 #4 (`convertToInternalTileConfig` comment vs `applyHeatmapDefaults` reality, see commit `946412ad`). - **Resolved in #2199**: P0/P1 #1 (heatmap arm in `mcpTilesParam`) already shipped; P0/P1 #3 (heatmap entry in `buildQueryGuidePrompt` + `buildCreateDashboardPrompt`) added in commit `e583fc68`. - **Pending in #2199 post-rebase**: P0/P1 #2 (MCP `createDashboard`/`updateDashboard` calling `getHeatmapTilesWithIncompatibleSources`), since the helper is added in this PR and not yet importable from #2199's branch. - **Deferred (P2)**: heatmap GET fallthrough silently rewriting to line on malformed docs, and source-kind-change wedge on PUT. Both tracked in #2236.
| y: 0, | ||
| w: 12, | ||
| h: 4, | ||
| config: { |
There was a problem hiding this comment.
does it work to do this (pseudo code):
const config = {...}
// save_dashboard call
const saved = ...
expect(saved.tiles[0].config).toMatchObject(config)
that type of pattern would reduce the duplicate code in this test significantly. (duplicate config object 6 times in this test: set, get config, get select x2 for update)
Brandon flagged on #2199 that the schema unit tests only assert that zod accepts the shape and don't prove that an agent-issued hyperdx_save_dashboard call actually persists a heatmap with every MCP-specific field. The bare-minimum heatmap test that lands with the #2200 heatmap external-API work covers only the required valueExpression; this test drives the full round-trip with heatmapScaleType, countExpression, numberFormat, where, and whereLanguage all set, then mutates them on PUT and re-reads to confirm the new values stick. Drive-by: drops an em-dash from a sibling test comment to satisfy the prose-lint hook on this file.
73aaf3b to
9b59aa5
Compare
|
@alex-fedotyev the latest commit didn't change anything |
Deep Review✅ No critical issues found. This PR is a test-only addition (~234 lines in 🔵 P3 nitpicks (1)
Reviewers (7): correctness, testing, maintainability, project-standards, agent-native, learnings, kieran-typescript. Testing gaps:
|
…forward fields Addresses Brandon's review feedback: - Extract createConfig / updatedConfig constants and use toMatchObject against them on every read. The literal heatmap config previously appeared 6 times across save / get / update / refetch assertions; now it appears twice (once as the create input, once as the mutation) and the four expect blocks each read one variable. - Drop the wall-of-text comment above the it(). The test name carries the intent. - Fix self-referential prose (the previous comment said "Brandon flagged on #2199" inside #2199). Also addresses deep-review P2: the update payload spreads fetched.tiles[0].config so numberFormat, whereLanguage, and sourceId are carried forward. Re-asserting toMatchObject(updatedConfig) on both updated and refetched now catches a regression where the PUT path silently drops any of those carry-forward fields, which the prior assertions (only on select and where) would have missed.
…ile round-trip Adds three integration tests that close coverage holes Brandon flagged: 1. Update changing displayType to heatmap on a non-Trace source: the existing reject test covers create-side only. This exercises the update path through filterChangedHeatmapTiles' "displayType changed to heatmap" branch. 2. Update changing a heatmap tile's source to a non-Trace source: exercises filterChangedHeatmapTiles' "sourceId changed on existing heatmap" branch. Together with #1, both update-side gate conditions in the OSS PR's filterChangedHeatmapTiles are now reachable from a test (previously zero coverage). 3. Mixed heatmap + line + number tiles in one dashboard: catches regressions where a serializer/deserializer change accidentally drops or clobbers sibling tile configs. Each new test is verified to detect a real regression via local mutation testing: - Short-circuiting filterChangedHeatmapTiles to return [] causes exactly tests #1 and #2 to fail (2 failed, 6 passed of the 8 heatmap-name tests). With the function restored, all 8 pass. - Replacing tiles.map with tiles.slice(0, 1).map in convertExternalTilesToInternal causes exactly test #3 to fail (1 failed, 7 passed). With map restored, all 8 pass. Test naming, structure, and config-constant pattern follow the toMatchObject style Brandon asked for; no wall-of-text comments.
Summary
Adds a thorough save → get → update → get round-trip test for the
heatmap MCP path. Brandon flagged on this PR that the schema unit
tests at
mcp/tools/dashboards/__tests__/schemas.test.tsonly assertthat zod accepts the shape and don't prove that an agent-issued
hyperdx_save_dashboardcall actually persists a heatmap with everyMCP-specific field. The bare-minimum heatmap test that landed with
#2200 covers only the required
valueExpression; this testexercises
heatmapScaleType,countExpression,numberFormat,where, andwhereLanguageall at once, then mutates them on PUTand re-reads to confirm the new values stick.
Backstory
The MCP heatmap schema work originally drafted on this branch
(
mcpHeatmapSelectItemSchema,mcpHeatmapTileSchema, the few-shotexample, the prompt guides) was absorbed into #2200 during the
convergence on the deep-review feedback. #2200 has merged on
main,so the only delta this PR adds on top is the integration test.
What's in this PR
packages/api/src/mcp/__tests__/dashboards.test.ts:should round-trip a fully-specified heatmap tile through save, get, and updatesame file so the prose-lint hook passes when this file is touched
Test plan
yarn workspace @hyperdx/api lintcleanyarn workspace @hyperdx/api tsc --noEmitcleanyarn workspace @hyperdx/api docgenproduces no openapi.json diffmake dev-intneedsDocker BuildKit which isn't available on this host.